658 snippet support sqlcmd #721

AnirudhVIyer · 2023-07-11T17:28:01Z

Describe your changes

Added suport for snippets in explore and profile commands.
In each command now we check if the table name is a reference to a table or a snippet
Use an internal variable with_clause that changes the SQL query enabling support for snippets in these commands.

Issue number

Closes #658

Checklist before requesting a review

Performed a self-review of my code
Formatted my code with pkgmt format
Added tests (when necessary).
Added docstring documentation and update the changelog (when needed)

📚 Documentation preview 📚: https://jupysql--721.org.readthedocs.build/en/721/

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1204895014778486

fix ipynb

edublancas

minor comments. I tested it and it works well

edublancas · 2023-07-12T00:02:12Z

src/sql/inspect.py

@@ -173,7 +174,7 @@ class Columns(DatabaseInspection):
    """

    def __init__(self, name, schema, conn=None) -> None:
-        util.is_table_exists(name, schema)
+        util._check_table_exists(name, schema)


so I see you added logic here that will let this continue if name is defined as a snippet. I think that makes sense so this feature works but I wonder if it'll have any side-effects since we're using this class in multiple places

@yafimvo, thoughts?

I think it makes sense because if the table exists so the following actions (listing columns, performing queries, etc.) should work. I also see it passes the integration and questdb tests (profile and columns) so I guess there are no negative side effects here.

Hi, the _check_table_exists function first checks if the table is a snippet name, and if it is not we run the is_table_exists inside the earlier function. So, if we pass a table name that is neither a snippet or a table, this will work similar to the check we had before.

src/sql/util.py

src/tests/test_magic_cmd.py

edublancas · 2023-07-12T00:08:02Z

@AnirudhVIyer please fix merge conflicts in the changelog

yafimvo

@AnirudhVIyer
Added some comments

yafimvo · 2023-07-12T07:52:16Z

src/sql/inspect.py

@@ -173,7 +174,7 @@ class Columns(DatabaseInspection):
    """

    def __init__(self, name, schema, conn=None) -> None:
-        util.is_table_exists(name, schema)
+        util._check_table_exists(name, schema)


I think it makes sense because if the table exists so the following actions (listing columns, performing queries, etc.) should work. I also see it passes the integration and questdb tests (profile and columns) so I guess there are no negative side effects here.

src/sql/util.py

src/tests/test_magic_cmd.py

doc/user-guide/table_explorer.ipynb

neelasha23 · 2023-07-12T10:35:59Z

This is displaying plotting using saved snippet .. maybe we should change this to a more generic message? Also, it's coming up twice

src/sql/inspect.py

AnirudhVIyer · 2023-07-12T17:41:04Z

This is displaying `plotting using saved snippet` .. maybe we should change this to a more generic message? Also, it's coming up twice

fixed this

…dhVIyer/jupysql into 658-snippet-support-sqlcmd merge origin

src/sql/util.py

src/tests/test_magic_cmd.py

neelasha23 · 2023-07-14T05:19:04Z

src/sql/util.py

+        name of the snippet
+    """
+    with_ = None
+    if is_saved_snippet(table, task):


I think task shouldn't be a parameter of is_saved_snippet. Can refactor this a bit:

if is_saved_snippet(table): print(f"{msg} using saved snippet : {table}")

And delete the print statement inside is_saved_snippet

Made this change, now we send a message in is_saved_snippet_or_table_exists itself.

@neelasha23 why do we need a message at all? The users already know the action they are performing (since they typed:%sqlcmd explore or %sqlcmd profile) and they know it's a snippet (if not, does it matter?).

The message was added as part of the CTE deprecation issue. initially it was only plotting.. it was needed to clarify that we are plotting using saved snippet. @yafimvo

If it's clear anyways then I think we can remove it

Sure! removing it

AnirudhVIyer · 2023-07-20T22:14:18Z

@edublancas

@AnirudhVIyer the tests in master are passing so it's either an error triggered by this PR or some change in a dependency, you'll have to do some debugging. maybe a duckdb or duckdb-engine update?

The errors are fixed.

neelasha23

looks good!

edublancas · 2023-07-26T17:32:29Z

@AnirudhVIyer this PR has lots of conflicts now because we merge a PR that refactored a lot of things. check how difficult it is to fix the merge conflicts, alternatively, you can create a new PR and manually apply your changes

AnirudhVIyer · 2023-07-27T20:03:03Z

@AnirudhVIyer this PR has lots of conflicts now because we merge a PR that refactored a lot of things. check how difficult it is to fix the merge conflicts, alternatively, you can create a new PR and manually apply your changes

@edublancas fixed the Merge conflicts

edublancas

please fix tests. I think a lot of the will fail because the .execute and ._prepare_query methods are a bit inconsistent (I'll work on a PR soon)

in the meantime, please address the observations in the tests, if some of the databases fail. it's fine, you can add a pytest.param(..., mark=pytest.mark.xfail(..)) (see existing tests to see how) this will allow the test to fail.

as long as duckdb sqlite and postgres pass, we can check the other databases one by one and see what's going on

edublancas · 2023-07-28T04:38:22Z

src/tests/integration/test_generic_db_operations.py

+    ip_with_dynamic_db = request.getfixturevalue(ip_with_dynamic_db)
+    ip_with_dynamic_db.run_cell(
+        """
+    %%sql sqlite://


if you pass sqlite:// you'll overwrite the db and you'll be testing with sqlite instead of the database from the ip_with_dynamic_db, please remove

src/tests/test_magic_cmd.py

AnirudhVIyer · 2023-07-28T16:27:48Z

please fix tests. I think a lot of the will fail because the .execute and ._prepare_query methods are a bit inconsistent (I'll work on a PR soon)

in the meantime, please address the observations in the tests, if some of the databases fail. it's fine, you can add a pytest.param(..., mark=pytest.mark.xfail(..)) (see existing tests to see how) this will allow the test to fail.

as long as duckdb sqlite and postgres pass, we can check the other databases one by one and see what's going on

got it! making the changes

edublancas · 2023-08-05T00:22:13Z

cool, don't forget to request a review when ready

edublancas · 2023-08-08T23:10:47Z

src/tests/integration/test_generic_db_operations.py

+    if ip_with_dynamic_db not in [
+        "ip_with_postgreSQL",
+        "ip_with_duckDB",
+        "ip_with_SQLite",
+    ]:
+        pytest.xfail(reason="There may be an issue due to code refactoring")


let the tests run and only exclude the ones where we are sure they'll fail

edublancas · 2023-08-08T23:11:26Z

src/tests/integration/test_generic_db_operations.py

+    %%sql
+    CREATE TABLE people (name varchar(50), number int, country varchar(50));
+    INSERT INTO people VALUES ('joe', 82, 'usa');
+    INSERT INTO people VALUES ('paula', 93, 'uk');
+    INSERT INTO people VALUES ('sam', 77, 'canada');
+    INSERT INTO people VALUES ('emily', 65, 'usa');
+    INSERT INTO people VALUES ('michael', 89, 'usa');
+    INSERT INTO people VALUES ('sarah', 81, 'uk');
+    INSERT INTO people VALUES ('james', 76, 'usa');
+    INSERT INTO people VALUES ('angela', 88, 'usa');
+    INSERT INTO people VALUES ('robert', 82, 'usa');
+    INSERT INTO people VALUES ('lisa', 92, 'uk');
+    INSERT INTO people VALUES ('mark', 77, 'usa');
+    INSERT INTO people VALUES ('jennifer', 68, 'australia');
+    """
+    )
+    ip_with_dynamic_db.run_cell(
+        """
+        %%sql --save citizen
+        select * from people where country = 'usa'
+        """
+    )


we're already creating some tables for all databases (although they have different names), check out how we're getting the table names. there's no need to create new tables here

edublancas · 2023-08-08T23:12:18Z

src/tests/integration/test_generic_db_operations.py

+        select * from people where country = 'usa'
+        """
+    )
+    with capture_output() as captured:


we're using pytests capsys for this. it works pretty much the same, let's replace it so our tests are consistent

edublancas · 2023-08-08T23:12:46Z

src/tests/test_magic_cmd.py

+        %%sql
+        CREATE TABLE people (name varchar(50), number int, country varchar(50));
+        INSERT INTO people VALUES ('joe', 82, 'usa');
+        INSERT INTO people VALUES ('paula', 93, 'uk');
+        INSERT INTO people VALUES ('sam', 77, 'canada');
+        INSERT INTO people VALUES ('emily', 65, 'usa');
+        INSERT INTO people VALUES ('michael', 89, 'usa');
+        INSERT INTO people VALUES ('sarah', 81, 'uk');
+        INSERT INTO people VALUES ('james', 76, 'usa');
+        INSERT INTO people VALUES ('angela', 88, 'usa');
+        INSERT INTO people VALUES ('robert', 82, 'usa');
+        INSERT INTO people VALUES ('lisa', 92, 'uk');
+        INSERT INTO people VALUES ('mark', 77, 'usa');
+        INSERT INTO people VALUES ('jennifer', 68, 'australia');
+        """
+    )


use existing tables

edublancas · 2023-08-08T23:13:29Z

src/tests/test_magic_cmd.py

+    for row in stats_table:
+        profile_metric = _get_row_string(row, " ")
+        name = _get_row_string(row, "name")
+        number = _get_row_string(row, "number")
+        country = _get_row_string(row, "country")
+
+        assert profile_metric in expected
+        assert name == str(expected[profile_metric][0])
+        assert number == str(expected[profile_metric][1])
+        assert country == str(expected[profile_metric][2])


keep tests simples, using for or if makes them harder to understand. try simplifying this while removing this for loop

edublancas · 2023-08-08T23:13:59Z

src/tests/test_magic_cmd.py

+        %%sql
+        CREATE TABLE people (name varchar(50), number int, country varchar(50));
+        INSERT INTO people VALUES ('joe', 82, 'usa');
+        INSERT INTO people VALUES ('paula', 93, 'uk');
+        INSERT INTO people VALUES ('sam', 77, 'canada');
+        INSERT INTO people VALUES ('emily', 65, 'usa');
+        INSERT INTO people VALUES ('michael', 89, 'usa');
+        INSERT INTO people VALUES ('sarah', 81, 'uk');
+        INSERT INTO people VALUES ('james', 76, 'usa');
+        INSERT INTO people VALUES ('angela', 88, 'usa');
+        INSERT INTO people VALUES ('robert', 82, 'usa');
+        INSERT INTO people VALUES ('lisa', 92, 'uk');
+        INSERT INTO people VALUES ('mark', 77, 'usa');
+        INSERT INTO people VALUES ('jennifer', 68, 'australia');
+        """
+    )


use existing tables

AnirudhVIyer added 3 commits July 11, 2023 13:24

snippet support for profile and explore

a40bd59

snippet support for profile and explore 1

120ccdb

Update table_explorer.ipynb

f2deca6

fix ipynb

AnirudhVIyer marked this pull request as ready for review July 11, 2023 21:52

AnirudhVIyer requested review from edublancas, neelasha23 and yafimvo as code owners July 11, 2023 21:52

edublancas requested changes Jul 12, 2023

View reviewed changes

yafimvo requested changes Jul 12, 2023

View reviewed changes

neelasha23 reviewed Jul 12, 2023

View reviewed changes

doc/user-guide/table_explorer.ipynb Outdated Show resolved Hide resolved

neelasha23 reviewed Jul 12, 2023

View reviewed changes

src/sql/inspect.py Outdated Show resolved Hide resolved

Merge branch 'master' into 658-snippet-support-sqlcmd

e9251f7

AnirudhVIyer added 6 commits July 12, 2023 14:02

review changes 2

fc11cc4

Update table_explorer.ipynb

ddb7cbf

fix lint

44bdcda

Merge branch '658-snippet-support-sqlcmd' of https://github.com/Aniru…

62c5b83

…dhVIyer/jupysql into 658-snippet-support-sqlcmd merge origin

fix errors

023d366

fix errors review

901a2c4

AnirudhVIyer requested review from neelasha23, yafimvo and edublancas July 12, 2023 21:06

neelasha23 reviewed Jul 13, 2023

View reviewed changes

src/sql/util.py Show resolved Hide resolved

neelasha23 reviewed Jul 13, 2023

View reviewed changes

src/tests/test_magic_cmd.py Outdated Show resolved Hide resolved

modyfying snippet message

e9aabba

AnirudhVIyer requested a review from neelasha23 July 13, 2023 19:05

neelasha23 reviewed Jul 14, 2023

View reviewed changes

refactored is_snippet and with-clause

aea58f5

AnirudhVIyer requested a review from neelasha23 July 23, 2023 16:54

edublancas requested a review from yafimvo July 24, 2023 03:29

yafimvo previously approved these changes Jul 24, 2023

View reviewed changes

neelasha23 previously approved these changes Jul 24, 2023

View reviewed changes

Merge branch 'master' into 658-snippet-support-sqlcmd

8919b22

AnirudhVIyer dismissed stale reviews from neelasha23 and yafimvo via 8919b22 July 27, 2023 18:18

AnirudhVIyer added 5 commits July 27, 2023 15:06

refactor changes

4f98ffd

resolve merge conflicts

1e044ac

resolve merge conflicts 1

46189c0

resolve merge conflicts 2

b05c54e

resolve merge conflicts 3

9fe8de6

AnirudhVIyer added 3 commits July 27, 2023 17:46

Merge branch 'master' into 658-snippet-support-sqlcmd

8507eca

resolve merge conflicts 3

056c876

merge conflict 4

04b77a3

edublancas requested changes Jul 28, 2023

View reviewed changes

AnirudhVIyer added 2 commits July 31, 2023 12:28

changes

213757f

Merge branch 'master' into 658-snippet-support-sqlcmd

0194b32

AnirudhVIyer added 4 commits August 7, 2023 15:45

update tests

f0feedb

update tests

e3d2889

test commit

b28fed3

change integration test

e1049c1

AnirudhVIyer requested a review from edublancas August 8, 2023 18:46

edublancas requested changes Aug 8, 2023

View reviewed changes

edublancas closed this Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

658 snippet support sqlcmd #721

658 snippet support sqlcmd #721

AnirudhVIyer commented Jul 11, 2023 •

edited

Loading

edublancas left a comment

edublancas Jul 12, 2023

yafimvo Jul 12, 2023

AnirudhVIyer Jul 12, 2023

edublancas commented Jul 12, 2023

yafimvo left a comment •

edited

Loading

yafimvo Jul 12, 2023

neelasha23 commented Jul 12, 2023

AnirudhVIyer commented Jul 12, 2023

neelasha23 Jul 14, 2023

AnirudhVIyer Jul 14, 2023

yafimvo Jul 16, 2023

neelasha23 Jul 18, 2023 •

edited

Loading

neelasha23 Jul 18, 2023

AnirudhVIyer Jul 18, 2023

AnirudhVIyer commented Jul 20, 2023

neelasha23 left a comment

edublancas commented Jul 26, 2023

AnirudhVIyer commented Jul 27, 2023

edublancas left a comment

edublancas Jul 28, 2023

AnirudhVIyer commented Jul 28, 2023

edublancas commented Aug 5, 2023

edublancas Aug 8, 2023

edublancas Aug 8, 2023

edublancas Aug 8, 2023

edublancas Aug 8, 2023

edublancas Aug 8, 2023

edublancas Aug 8, 2023

658 snippet support sqlcmd #721

658 snippet support sqlcmd #721

Conversation

AnirudhVIyer commented Jul 11, 2023 • edited Loading

Describe your changes

Issue number

Checklist before requesting a review

edublancas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edublancas commented Jul 12, 2023

yafimvo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neelasha23 commented Jul 12, 2023

AnirudhVIyer commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neelasha23 Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirudhVIyer commented Jul 20, 2023

neelasha23 left a comment

Choose a reason for hiding this comment

edublancas commented Jul 26, 2023

AnirudhVIyer commented Jul 27, 2023

edublancas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirudhVIyer commented Jul 28, 2023

edublancas commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirudhVIyer commented Jul 11, 2023 •

edited

Loading

yafimvo left a comment •

edited

Loading

neelasha23 Jul 18, 2023 •

edited

Loading